feat: use runtime Gradle version during plugin AAR build #3731

DimitarTachev · 2018-07-05T13:39:48Z

PR Checklist

The PR title follows our guidelines: https://github.com/NativeScript/NativeScript/blob/master/CONTRIBUTING.md#commit-messages.
There is an issue for the bug/feature this PR is for. To avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it.
You have signed the CLA.
All existing tests are passing: https://github.com/NativeScript/nativescript-cli/blob/master/CONTRIBUTING.md#contribute-to-the-code-base
Tests for the changes are included.

Related to #3719

…kage.json of the runtime instead of hardcoded ones

… specified in the package.json of the runtime instead of hardcoded ones

…ersions

Fatme · 2018-07-06T08:10:23Z

lib/services/android-plugin-build-service.ts

+		for (const dir of androidSourceSetDirectories) {
+			const dirNameParts = dir.split(path.sep);
+			// get only the last subdirectory of the entire path string. e.g. 'res', 'java', etc.
+			const dirName = dirNameParts[dirNameParts.length - 1];


You can use path.dirname(path.dirname(dir))

after a further looking into that, it could be just path.basename(dir)

Fatme · 2018-07-06T11:31:28Z

lib/services/android-plugin-build-service.ts

-				const includeGradleContent = this.$fs.readText(includeGradlePath);
-				const repositoriesAndDependenciesScopes = this.getIncludeGradleCompileDependenciesScope(includeGradleContent);
+	private replaceGradleVersion(pluginTempDir: string, version: string): void {
+		const gradleVersion = version || "4.4";


4.4 can be extracted to constant

Fatme · 2018-07-06T11:32:36Z

lib/services/android-plugin-build-service.ts

-				}
+	private replaceGradleAndroidPluginVersion(buildGradlePath: string, version: string): void {
+		const gradleAndroidPluginVersionPlaceholder = "{{runtimeAndroidPluginVersion}}";
+		const gradleAndroidPluginVersion = version || "3.1.2";


Also can be extracted to constant - 3.1.2

Fatme · 2018-07-06T11:35:17Z

lib/services/android-plugin-build-service.ts

+		const includeGradlePath = path.join(platformsAndroidDirPath, INCLUDE_GRADLE_NAME);
+		if (this.$fs.exists(includeGradlePath)) {
+			const includeGradleContent = this.$fs.readText(includeGradlePath);
+			const repositoriesAndDependenciesScopes = this.getIncludeGradleCompileDependenciesScope(includeGradleContent);


Maybe compileDependencies is a better name. It is not a good practice to have and in the name.

Fatme · 2018-07-06T12:11:10Z

lib/services/android-plugin-build-service.ts

+				androidManifestContent = this.$fs.readText(manifestFilePath);
+			} catch (err) {
+				throw new Error(
+					`Failed to fs.readFileSync the manifest file located at ${manifestFilePath}`


It will be nice to add the message from real error here.

`Failed to fs.readFileSync the manifest file located at ${manifestFilePath}`

Also use $errors.failWithoutHelp

Fatme · 2018-07-06T12:15:05Z

lib/services/android-plugin-build-service.ts

+		try {
+			this.$fs.writeFile(pathToTempAndroidManifest, updatedManifestContent);
+		} catch (e) {
+			throw new Error(`Failed to write the updated AndroidManifest in the new location - ${pathToTempAndroidManifest}`);


Also we can add the real error message to the output.
Use this.$errors.fail

Fatme · 2018-07-06T12:17:22Z

lib/services/android-plugin-build-service.ts

+		const packageJsonGradle = packageData && packageData.gradle;
+		let runtimeVersions: IRuntimeGradleVersions = null;
+		if (packageJsonGradle && (packageJsonGradle.version || packageJsonGradle.android)) {
+			runtimeVersions = {};


We can remove this line and initialize the variable when declaring it.

Its here as we want to return null in such cases

Fatme · 2018-07-06T12:20:43Z

lib/services/android-plugin-build-service.ts

+			const repositoriesAndDependenciesScopes = this.getIncludeGradleCompileDependenciesScope(includeGradleContent);
+
+			if (repositoriesAndDependenciesScopes.length > 0) {
+				this.$fs.appendFile(buildGradlePath, "\n" + repositoriesAndDependenciesScopes.join("\n"));


Maybe EOL is better than \n

Fatme · 2018-07-06T12:22:10Z

lib/services/android-plugin-build-service.ts

+					this.$fs.copyFile(pathToBuiltAar, path.join(aarOutputDir, `${shortPluginName}.aar`));
+				}
+			} catch (e) {
+				throw new Error(`Failed to copy built aar to destination. ${e.message}`);


Also add the real error message to the output.

Fatme · 2018-07-07T06:18:36Z

lib/node-package-manager.ts

@@ -123,7 +123,7 @@ export class NodePackageManager implements INodePackageManager {
 		const url = `https://registry.npmjs.org/${packageName}`;
 		this.$logger.trace(`Trying to get data from npm registry for package ${packageName}, url is: ${url}`);
 		const responseData = (await this.$httpClient.httpRequest(url)).body;
-		this.$logger.trace(`Successfully received data from npm registry for package ${packageName}. Response data is: ${responseData}`);
+		this.$logger.trace(`Successfully received data from npm registry for package ${packageName}.`);
 		const jsonData = JSON.parse(responseData);


Why do you delete response data from the log message? It is added here with debug purposes?

Fatme · 2018-07-07T06:20:20Z

lib/services/android-plugin-build-service.ts

@@ -164,113 +169,157 @@ export class AndroidPluginBuildService implements IAndroidPluginBuildService {
 	 */
 	public async buildAar(options: IBuildOptions): Promise<boolean> {
 		this.validateOptions(options);
+		const manifestFilePath = this.getManifest(options.platformsAndroidDirPath);
+		const androidSourceSetDirectories = this.getAndroidSourceDirectories(options.platformsAndroidDirPath);


Maybe androidSourceDirectories is a better name.

Fatme · 2018-07-09T06:15:35Z

lib/services/android-plugin-build-service.ts

+				androidManifestContent = this.$fs.readText(manifestFilePath);
+			} catch (err) {
+				throw new Error(
+					`Failed to fs.readFileSync the manifest file located at ${manifestFilePath}`


Use this.$errors.fail or this.$erros.failWithoutHelp instead throw new Error

Fatme · 2018-07-09T06:24:48Z

lib/services/android-plugin-build-service.ts

+			const includeGradleContent = this.$fs.readText(includeGradlePath);
+			const repositoriesAndDependenciesScopes = this.getIncludeGradleCompileDependenciesScope(includeGradleContent);
+
+			if (repositoriesAndDependenciesScopes.length > 0) {


repositoriesAndDependenciesScopes.length is enough

Fatme · 2018-07-09T06:25:51Z

lib/services/android-plugin-build-service.ts

+				throw new Error(`Failed to copy built aar to destination. ${e.message}`);
+			}
+		} else {
+			throw new Error(`No built aar found at ${pathToBuiltAar}`);


errors.fail()

Fatme · 2018-07-09T06:30:14Z

test/services/android-plugin-build-service.ts

+		};
+	}
+
+	function setupDI(options: {


Maybe createTestInjector is a better name

dtopuzov · 2018-07-09T07:56:58Z

run ci

Natalia-Hristova · 2018-07-09T15:33:37Z

run ci

Natalia-Hristova · 2018-07-09T15:39:58Z

run ci

rosen-vladimirov

That's really great work!

rosen-vladimirov · 2018-07-09T18:53:32Z

lib/definitions/android-plugin-migrator.d.ts

@@ -33,5 +34,5 @@ interface IBuildAndroidPluginData {
 	/**
 	 * Information about tools that will be used to build the plugin, for example compile SDK version, build tools version, etc.
 	 */
-	androidToolsInfo: IAndroidToolsInfoData;
+	androidToolsInfo?: IAndroidToolsInfoData;


why this is not mandatory anymore?

We are using it in a single place and now it defaults to the old logic inside (this.$androidToolsInfo.getToolsInfo();)

Natalia-Hristova · 2018-07-10T07:33:07Z

run ci

Fatme

Great work!!

Dimitar Tachev added 6 commits July 5, 2018 16:31

refactor: make the buildAar method shorter and more readable

983c3b7

feat: use the Gradle and Android plugin versions specified in the pac…

543ded6

…kage.json of the runtime instead of hardcoded ones

test: add unit tests for using the Gradle and Android plugin versions…

9ae5b0a

… specified in the package.json of the runtime instead of hardcoded ones

refactor: make the buildAar tests more readable

9dcffbe

chore: add some trace logging when searching for the runtime Gradle v…

86f7f38

…ersions

refactor: create a common injector stub

dcc78bf

DimitarTachev requested review from rosen-vladimirov, Fatme and KristianDD July 5, 2018 15:15

Fatme reviewed Jul 6, 2018

View reviewed changes

Natalia-Hristova added this to the 4.2.0 milestone Jul 6, 2018

Fatme suggested changes Jul 9, 2018

View reviewed changes

rosen-vladimirov approved these changes Jul 9, 2018

View reviewed changes

Fatme approved these changes Jul 10, 2018

View reviewed changes

DimitarTachev force-pushed the tachev/use-runtime-gradle-version branch 3 times, most recently from ad8fe7f to 4f46b6a Compare July 10, 2018 11:33

chore: fix pull request comments

1a881a1

DimitarTachev force-pushed the tachev/use-runtime-gradle-version branch from 4f46b6a to 1a881a1 Compare July 10, 2018 11:53

DimitarTachev merged commit 42e4210 into master Jul 10, 2018

DimitarTachev deleted the tachev/use-runtime-gradle-version branch July 10, 2018 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use runtime Gradle version during plugin AAR build #3731

feat: use runtime Gradle version during plugin AAR build #3731

DimitarTachev commented Jul 5, 2018

Fatme Jul 6, 2018

DimitarTachev Jul 9, 2018

Fatme Jul 6, 2018

Fatme Jul 6, 2018

Fatme Jul 6, 2018

Fatme Jul 6, 2018 •

edited

Loading

Fatme Jul 6, 2018 •

edited

Loading

Fatme Jul 6, 2018

DimitarTachev Jul 10, 2018

Fatme Jul 6, 2018

Fatme Jul 6, 2018

Fatme Jul 7, 2018

Fatme Jul 7, 2018

Fatme Jul 9, 2018

Fatme Jul 9, 2018

Fatme Jul 9, 2018

Fatme Jul 9, 2018

dtopuzov commented Jul 9, 2018

Natalia-Hristova commented Jul 9, 2018

Natalia-Hristova commented Jul 9, 2018

rosen-vladimirov left a comment

rosen-vladimirov Jul 9, 2018

DimitarTachev Jul 10, 2018

Natalia-Hristova commented Jul 10, 2018

Fatme left a comment

feat: use runtime Gradle version during plugin AAR build #3731

feat: use runtime Gradle version during plugin AAR build #3731

Conversation

DimitarTachev commented Jul 5, 2018

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fatme Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

Fatme Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtopuzov commented Jul 9, 2018

Natalia-Hristova commented Jul 9, 2018

Natalia-Hristova commented Jul 9, 2018

rosen-vladimirov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Natalia-Hristova commented Jul 10, 2018

Fatme left a comment

Choose a reason for hiding this comment

Fatme Jul 6, 2018 •

edited

Loading

Fatme Jul 6, 2018 •

edited

Loading